ProviderManager should have a varargs constructor #7806
Conversation
spring-projects-issues
added
the
status: waiting-for-triage
| @@ -96,7 +94,7 @@ public void authenticationSucceedsWhenFirstProviderReturnsNullButSecondAuthentic | |||
|
|
|||
| @Test(expected = IllegalArgumentException.class) | |||
| public void testStartupFailsIfProvidersNotSet() { | |||
| new ProviderManager(null); | |||
There was a problem hiding this comment.
I think we should still have a test for new ProviderManager((AuthenticationProvider) null) and for new ProviderManager((List<AuthenticationProvider>) null).
The reason is that an application might do:
AuthenticationProvider provider = constructProvider();
return new ProviderManager(provider);but constructProvider returns null.
I believe this also implies that the extra validation logic in checkState() will turn out to be necessary.
There was a problem hiding this comment.
Got it, thanks for the clarification. I have added the extra validation login in checkState() and tests for both null scenarios.
| @@ -189,7 +186,7 @@ public void parentAuthenticationIsUsedIfProvidersDontAuthenticate() { | |||
| Authentication authReq = mock(Authentication.class); | |||
| when(parent.authenticate(authReq)).thenReturn(authReq); | |||
| ProviderManager mgr = new ProviderManager( | |||
| Arrays.asList(mock(AuthenticationProvider.class)), parent); | |||
| Collections.singletonList(mock(AuthenticationProvider.class)), parent); | |||
There was a problem hiding this comment.
Thanks for these cleanup items, too. Will you please put them in a separate commit (same PR)? That is, any code changes that aren't directly related to the new constructor, let's separate them into their own commit. This simplifies maintenance.
There was a problem hiding this comment.
Ok, now they are in their own commit.
jzheaux
added
in: core
|
@ThomasVitale Will you be able to apply the requested changes? |
|
@jzheaux I have just delivered the requested changes, sorry for the delay. Does it look ok now? |
- Added varargs constructor to ProviderManager. - Added check for null values in AuthenticationProvider list. - Updated ProviderManagerTests to test for null values using both constructors. Fixes gh-7713
|
Nice, @ThomasVitale! This is now merged into Also, I added a very slight polish via d22b476 |
Fixes gh-7713